CASSGO-11 Support vector type #1828

lukasz-antoniak · 2024-10-10T05:53:18Z

Complete vector type support for GoCQL. Supports fixed and variable-length vector types (e.g. vector of floats, vector of list of text).

marshal.go

tengu-alt · 2024-10-15T11:52:06Z

Have you tested it with protoV5? If not I would be glad to handle it with #1822.

marshal.go

joao-r-reis · 2024-10-15T14:09:46Z

Have you tested it with protoV5?

Hmm protocol v5 shouldn't matter much here, the encoding of the type itself is the same regardless of whether v4 or v5 is being used.

joao-r-reis

It's looking solid, I left a few comments. Also note that you left a few TODOs.

helpers.go

metadata_test.go

frame.go

metadata.go

helpers.go

joao-r-reis

I like it 👍 added a couple of small comments

helpers.go

metadata.go

joao-r-reis

Good stuff 👍

joao-r-reis · 2024-10-30T18:03:29Z

marshal.go

+		}
+		if k == reflect.Array {
+			if rv.Len() != info.Dimensions {
+				return unmarshalErrorf("unmarshal vector: array with wrong size")


NIT: maybe add the expected and provided sizes to the error message

joao-r-reis · 2024-10-30T18:04:51Z

vector_test.go

+		{name: "smallint", cqlType: TypeSmallInt.String(), value: []int16{127, 256, -1234}},
+		{name: "tinyint", cqlType: TypeTinyInt.String(), value: []int8{127, 9, -123}},
+		{name: "duration", cqlType: TypeDuration.String(), value: []Duration{duration1, duration2, duration3}},
+		// TODO(lantonia): Test vector of custom types


NIT: a couple of TODOs in this test still

joao-r-reis · 2024-10-30T18:06:15Z

.github/workflows/main.yml

          sleep 30s
      - name: Integration tests
        run: |
+          source ~/venv/bin/activate


just a reminder to remove the workflow changes from this PR before wrapping up (we'll have to potentially merge #1833 first) when we get a second +1

Yes, this PR will wait for #1833. I will remove workflow changes form this PR, rebase and squash commits.

jameshartig · 2025-01-25T22:12:30Z

I'd be curious your thoughts on if implementing this after #1855 would've made it easier.

joao-r-reis · 2025-03-27T15:02:22Z

I'd be curious your thoughts on if implementing this after #1855 would've made it easier.

@lukasz-antoniak can you explore this? I imagine that this implementation doesn't have much overlap with #1855 and I'd like to get it in before #1855 if this is the case but I'd like your thoughts on this @lukasz-antoniak

If we do want to get this done before #1855 then we need a rebase on this PR and then I can review it again

joao-r-reis

Looks good, I think we need to move the C* versions to 4.1.x and 5.0.x in CI so that we get coverage for these tests before we approve the PR to be merged

joao-r-reis · 2025-04-08T12:26:17Z

helpers_test.go

+			},
+		},
+		{
+			"vector<vector<float, 3>, 5>", VectorType{


It would be nice to have a couple of cases with more complex types like vector<map<uuid,timestamp>, 5> for example

Added two more - one with map, and other with frozen tuple.

worryg0d

Everything looks good to me, but we still need a couple of folks to review this one just to ensure we didn't miss something.

ribaraka · 2025-04-08T20:17:25Z

helpers.go

+			Elements:   fields,
+		}
+	} else if strings.HasPrefix(name, VECTOR_TYPE) {
+		names := splitJavaCompositeTypes(strings.TrimPrefix(name[:len(name)-1], VECTOR_TYPE+"("))


In the splitCQLCompositeTypes and splitJavaCompositeTypes, the prefix trimming logic is repeated in multiple cases. It could be moved inside those funcs, which would make the code cleaner and reduce repetition.

ribaraka · 2025-04-08T22:01:32Z

helpers.go


-func getCassandraType(name string, logger StdLogger) TypeInfo {
+// Parses long Java-style type definition to internal data structures.
+func getCassandraLongType(name string, protoVer byte, logger StdLogger) TypeInfo {


Should we add a TODO to cover getCassandraLongType with tests?

For now added TODO. This is tested indirectly in e.g. TestFunctionMetadata.

ribaraka · 2025-04-08T22:17:10Z

marshal.go

+	case TypeCustom:
+		switch info.(type) {
+		case VectorType:
+			return marshalVector(info.(VectorType), value)
+		}


you can simplify this block of code by assigning the type to var so you eliminate the need for the second type assertion:

Suggested change

case TypeCustom:

switch info.(type) {

case VectorType:

return marshalVector(info.(VectorType), value)

}

case TypeCustom:

switch t := info.(type) {

case VectorType:

return marshalVector(t, value)

}

}

Also the switch statement appears to only handle the VectorType case without any default case.

The nested switch looks redundant.
Maybe something like this would be more suitable?

case TypeCustom: if vector, ok := info.(VectorType); ok { return marshalVector(vector, value) }

ribaraka · 2025-04-08T22:19:34Z

marshal.go

+	case TypeCustom:
+		switch info.(type) {
+		case VectorType:
+			return unmarshalVector(info.(VectorType), data, value)
+		}


same as above

ribaraka · 2025-04-08T22:41:48Z

marshal.go

+		}
+		return buf.Bytes(), nil
+	}
+	return nil, marshalErrorf("can not marshal %T into %s", value, info)


I think it would be helpful to mention which types are actually marshalable (e.g., array, slice)

The message is in-line with others. Do we want to make an exception?

There is actually a messadges with the provided accepted types like in here

ribaraka · 2025-04-08T22:44:59Z

marshal.go

+		}
+		return nil
+	}
+	return unmarshalErrorf("can not unmarshal %s into %T", info, value)


same as above

ribaraka · 2025-04-08T23:13:46Z

marshal.go

+			if isVectorVariableLengthType(info.SubType) {
+				writeUnsignedVInt(buf, uint64(len(item)))


nitpick: These functions could use some documentation imo—particularly explaining how it works, the intended purpose, and what happens if info.SubType is not isVectorVariableLengthType.

When dealing with fix-length types, serializing length of the element is not required. isVectorVariableLengthType returns true for all types that require serialization of their length. Not sure what comment should be appropriate, which does not just read the code.

Thanks for the explanation — that clarifies things. I was curios how data considered "Vector Variable Length" in this case. So if you like it, I suggest adding comment as following:

// isVectorVariableLengthType determines if a type requires explicit length serialization within a vector. Variable-length types need their length encoded before the actual data to allow proper deserialization. Fixed-length types, on the other hand, don't require this kind of length prefix.

Also, if there are no serialization requirements for fixed-length types, should TypeSmallInt and TypeTinyInt be removed from isVectorVariableLengthType?

Added your comment. Unfortunately for those types server expects the length also.

ribaraka · 2025-04-08T23:17:16Z

marshal.go

+	switch elemType.Type() {
+	case TypeVarchar, TypeAscii, TypeBlob, TypeText:
+		return true
+	case TypeCounter:
+		return true
+	case TypeDuration, TypeDate, TypeTime:
+		return true
+	case TypeDecimal, TypeSmallInt, TypeTinyInt, TypeVarint:
+		return true
+	case TypeInet:
+		return true
+	case TypeList, TypeSet, TypeMap, TypeUDT, TypeTuple:
+		return true


why don't combine it in one return?

You mean that instead all of this cases could be single "if", or one case for all suitable types?
As I can see, it's structured by similar types for better readability, not sure that it should be merged into one.

Readability is the reason.

Just showing what you said no to 🤣

switch elemType.Type() { case TypeVarchar, TypeAscii, TypeBlob, TypeText, TypeCounter, TypeDuration, TypeDate, TypeTime, TypeDecimal, TypeSmallInt, TypeTinyInt, TypeVarint, TypeInet, TypeList, TypeSet, TypeMap, TypeUDT, TypeTuple: return true

This looks like a discussion about { put always on new line in C* code base. Changed as you wish.

I wasn't insisting, just sharing a visual.

OleksiienkoMykyta · 2025-04-09T07:46:19Z

marshal.go

+	case TypeCustom:
+		switch info.(type) {
+		case VectorType:
+			return marshalVector(info.(VectorType), value)
+		}


The nested switch looks redundant.
Maybe something like this would be more suitable?

case TypeCustom: if vector, ok := info.(VectorType); ok { return marshalVector(vector, value) }

OleksiienkoMykyta · 2025-04-09T08:05:19Z

marshal.go

+	switch elemType.Type() {
+	case TypeVarchar, TypeAscii, TypeBlob, TypeText:
+		return true
+	case TypeCounter:
+		return true
+	case TypeDuration, TypeDate, TypeTime:
+		return true
+	case TypeDecimal, TypeSmallInt, TypeTinyInt, TypeVarint:
+		return true
+	case TypeInet:
+		return true
+	case TypeList, TypeSet, TypeMap, TypeUDT, TypeTuple:
+		return true


You mean that instead all of this cases could be single "if", or one case for all suitable types?
As I can see, it's structured by similar types for better readability, not sure that it should be merged into one.

OleksiienkoMykyta · 2025-04-09T08:07:46Z

marshal.go

+	buf.Write(tmp)
+}
+
+func readUnsignedVint(data []byte, start int) (uint64, int, error) {


readUnsignedVint -> readUnsignedVInt, it would be a bit more consistent.

There is already encVint so maybe let us leave it as is?

It would be inconsistent with writeUnsignedVInt

OleksiienkoMykyta · 2025-04-09T08:13:01Z

marshal.go

+	buf.Write(tmp)
+}
+
+func readUnsignedVint(data []byte, start int) (uint64, int, error) {


Also, maybe it should be provided with unit tests, maybe even with fuzzing, to make sure that everything works properly.
Also, I think that it should be along with some documentation, to help developers dive into it faster.
Wdyt?

The description is in native protocol, so we do not need to duplicate it: https://github.com/apache/cassandra/blob/trunk/doc/native_protocol_v5.spec#L392C23-L399C45. Method signature already indicates what is the purpose. I can add a test.

tengu-alt · 2025-04-09T11:26:47Z

Overall, looks good to me, except this and this.

ribaraka · 2025-04-09T12:00:05Z

marshal.go

+		for i := 0; i < info.Dimensions; i++ {
+			offset := 0
+			if isVectorVariableLengthType(info.SubType) {
+				m, p, err := readUnsignedVint(data, 0)


The start parameter of the readUnsignedVint func seem to be redundunt. Could move inside the func. Or am I missing something?

It is but I would leave it as is when data will not represent just unsigned Vint, but longer slice. But if that is a problem, I can change it.

Hardcoding is kind of distracting. We can always add the parameter back when there's an actual requirement for it. I'd probably remove it if there is no use of it.
@joao-r-reis, I’d appreciate your thoughts on this when you have a moment.

Hardcoding is kind of distracting. We can always add the parameter back when there's an actual requirement for it. I'd probably remove it if there is no use of it. @joao-r-reis, I’d appreciate your thoughts on this when you have a moment.

Agree

ribaraka · 2025-04-09T12:04:09Z

marshal.go

+			if isVectorVariableLengthType(info.SubType) {
+				writeUnsignedVInt(buf, uint64(len(item)))


Thanks for the explanation — that clarifies things. I was curios how data considered "Vector Variable Length" in this case. So if you like it, I suggest adding comment as following:

// isVectorVariableLengthType determines if a type requires explicit length serialization within a vector. Variable-length types need their length encoded before the actual data to allow proper deserialization. Fixed-length types, on the other hand, don't require this kind of length prefix.

Also, if there are no serialization requirements for fixed-length types, should TypeSmallInt and TypeTinyInt be removed from isVectorVariableLengthType?

ribaraka · 2025-04-09T12:15:59Z

marshal.go

+	case TypeCustom:
+		switch elemType.(type) {
+		case VectorType:
+			vecType := elemType.(VectorType)
+			return isVectorVariableLengthType(vecType.SubType)
+		}
+		return true


since we agreed on similar switch statement, let's then keep it consistent 🙏

ctx: #1828 (comment)

Good catch. Changed to if statement like in other cases.

ribaraka

amazing job!

lukasz-antoniak · 2025-04-11T12:11:26Z

@joao-r-reis: Changed C* versions to run integration tests from 4.0 and 4.1, to 4.1 and 5.0.

joao-r-reis

Good stuff

joao-r-reis · 2025-04-11T14:25:34Z

I'll wait for approvals by @tengu-alt and @OleksiienkoMykyta before merging this

joao-r-reis · 2025-04-11T14:26:56Z

After they approve please squash the commits and use a commit description according to our guidelines

joao-r-reis · 2025-04-21T09:59:33Z

@tengu-alt @OleksiienkoMykyta do you have time to review/approve this PR this week?

OleksiienkoMykyta · 2025-04-21T10:04:39Z

@tengu-alt @OleksiienkoMykyta do you have time to review/approve this PR this week?

Sure, I'll check it this week

tengu-alt

Nice work, looks good to me!

OleksiienkoMykyta

LGTM. Thanks for the great contribution

joao-r-reis · 2025-04-22T12:15:15Z

@lukasz-antoniak can you squash and edit commit msg?

lukasz-antoniak · 2025-04-24T11:33:55Z

Done.

joao-r-reis · 2025-04-24T14:10:10Z

missing entry on CHANGELOG @lukasz-antoniak

Support marshalling and unmarshalling of vector custom type. patched by Lukasz Antoniak; reviewed by João Reis, Stanislav Bychkov, Oleksandr Luzhniy, Mykyta Oleksiienko and Bohdan Siryk for CASSGO-11

lukasz-antoniak mentioned this pull request Oct 10, 2024

CASSGO-11 Feature Request: Support Vector Type #1734

Closed

joao-r-reis mentioned this pull request Oct 14, 2024

WIP Vector support #1830

Closed

tengu-alt reviewed Oct 15, 2024

View reviewed changes

marshal.go Outdated Show resolved Hide resolved

tengu-alt reviewed Oct 15, 2024

View reviewed changes

marshal.go Outdated Show resolved Hide resolved

joao-r-reis reviewed Oct 22, 2024

View reviewed changes

joao-r-reis changed the title ~~Support vector type~~ CASSGO-11 Support vector type Oct 24, 2024

joao-r-reis reviewed Oct 29, 2024

View reviewed changes

metadata.go Outdated Show resolved Hide resolved

metadata.go Outdated Show resolved Hide resolved

helpers.go Outdated Show resolved Hide resolved

joao-r-reis reviewed Oct 30, 2024

View reviewed changes

helpers.go Outdated Show resolved Hide resolved

helpers.go Outdated Show resolved Hide resolved

helpers.go Outdated Show resolved Hide resolved

metadata.go Outdated Show resolved Hide resolved

joao-r-reis approved these changes Oct 30, 2024

View reviewed changes

joao-r-reis mentioned this pull request Jan 31, 2025

CASSGO-43: externally-defined type registration #1855

Merged

dkropachev mentioned this pull request Feb 20, 2025

Get vector search support in scylladb/gocql#401

Open

lukasz-antoniak force-pushed the 1734 branch from f9e22c7 to 50324e8 Compare April 7, 2025 18:09

joao-r-reis reviewed Apr 8, 2025

View reviewed changes

worryg0d approved these changes Apr 8, 2025

View reviewed changes

ribaraka reviewed Apr 8, 2025

View reviewed changes

OleksiienkoMykyta reviewed Apr 9, 2025

View reviewed changes

ribaraka reviewed Apr 9, 2025

View reviewed changes

ribaraka approved these changes Apr 11, 2025

View reviewed changes

joao-r-reis approved these changes Apr 11, 2025

View reviewed changes

tengu-alt approved these changes Apr 21, 2025

View reviewed changes

joao-r-reis mentioned this pull request Apr 21, 2025

CASSGO-71 Make HostFilter interface easier to test #1874

Closed

OleksiienkoMykyta approved these changes Apr 22, 2025

View reviewed changes

lukasz-antoniak force-pushed the 1734 branch from 5290f8e to 9d0df36 Compare April 24, 2025 11:33

CASSGO-11 Support vector type

bb21187

Support marshalling and unmarshalling of vector custom type. patched by Lukasz Antoniak; reviewed by João Reis, Stanislav Bychkov, Oleksandr Luzhniy, Mykyta Oleksiienko and Bohdan Siryk for CASSGO-11

lukasz-antoniak force-pushed the 1734 branch from 9d0df36 to bb21187 Compare April 24, 2025 14:18

joao-r-reis merged commit d32a392 into apache:trunk Apr 24, 2025
72 checks passed

dkropachev mentioned this pull request Oct 6, 2025

Merge vector support scylladb/gocql#585

Closed

		if isVectorVariableLengthType(info.SubType) {
		writeUnsignedVInt(buf, uint64(len(item)))

CASSGO-11 Support vector type #1828

CASSGO-11 Support vector type #1828

Uh oh!

Conversation

lukasz-antoniak commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tengu-alt commented Oct 15, 2024

Uh oh!

Uh oh!

joao-r-reis commented Oct 15, 2024

Uh oh!

joao-r-reis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joao-r-reis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joao-r-reis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jameshartig commented Jan 25, 2025

Uh oh!

joao-r-reis commented Mar 27, 2025

Uh oh!

joao-r-reis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

worryg0d left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lukasz-antoniak commented Oct 10, 2024 •

edited

Loading

ribaraka Apr 8, 2025 •

edited

Loading

ribaraka Apr 9, 2025 •

edited

Loading